Auto merge of #3136 - alexcrichton:warn-bad-override, r=brson
authorbors <bors@rust-lang.org>
Thu, 6 Oct 2016 00:40:11 +0000 (17:40 -0700)
committerGitHub <noreply@github.com>
Thu, 6 Oct 2016 00:40:11 +0000 (17:40 -0700)
Warn about path overrides that won't work

Cargo has a long-standing [bug] in path overrides where they will cause spurious
rebuilds of crates in the crate graph. This can be very difficult to diagnose
and be incredibly frustrating as well. Unfortunately, though, it's behavior that
fundamentally can't be fixed in Cargo.

The crux of the problem happens when a `path` replacement changes something
about the list of dependencies of the crate that it's replacing. This alteration
to the list of dependencies *cannot be tracked by Cargo* as the lock file was
previously emitted. In the best case this ends up causing random recompiles. In
the worst case it cause infinite registry updates that always result in
recompiles.

A solution to this intention, changing the dependency graph of an overridden
dependency, was [implemented] with the `[replace]` feature in Cargo awhile back.
With that in mind, this commit implements a *warning* whenever a bad dependency
replacement is detected. The message here is pretty wordy, but it's intended to
convey that you should switch to using `[replace]` for a more robust
impelmentation, and it can also give security to anyone using `path` overrides
that if they get past this warning everything should work as intended.

[bug]: https://github.com/rust-lang/cargo/issues/2041
[implemented]: http://doc.crates.io/specifying-dependencies.html#overriding-dependencies

Closes #2041

1  2 
src/cargo/core/registry.rs
tests/overrides.rs

index 99d9db83f95643f0ddc0437533726f998d2c7f70,ecaac98157bde33afd9d05c4ead761bce77d4d11..79089ee4d7efc66efcd437ab8c0e02e246874f37
@@@ -194,31 -192,30 +192,32 @@@ impl<'cfg> PackageRegistry<'cfg> 
          for s in self.overrides.iter() {
              let src = self.sources.get_mut(s).unwrap();
              let dep = Dependency::new_override(dep.name(), s);
-             ret.extend(try!(src.query(&dep)).into_iter().filter(|s| {
-                 seen.insert(s.name().to_string())
-             }));
+             let mut results = try!(src.query(&dep));
+             if results.len() > 0 {
+                 return Ok(Some(results.remove(0)))
+             }
          }
-         Ok(ret)
+         Ok(None)
      }
  
 -    // This function is used to transform a summary to another locked summary if
 -    // possible. This is where the concept of a lockfile comes into play.
 -    //
 -    // If a summary points at a package id which was previously locked, then we
 -    // override the summary's id itself, as well as all dependencies, to be
 -    // rewritten to the locked versions. This will transform the summary's
 -    // source to a precise source (listed in the locked version) as well as
 -    // transforming all of the dependencies from range requirements on imprecise
 -    // sources to exact requirements on precise sources.
 -    //
 -    // If a summary does not point at a package id which was previously locked,
 -    // we still want to avoid updating as many dependencies as possible to keep
 -    // the graph stable. In this case we map all of the summary's dependencies
 -    // to be rewritten to a locked version wherever possible. If we're unable to
 -    // map a dependency though, we just pass it on through.
 -    fn lock(&self, summary: Summary) -> Summary {
 +    /// This function is used to transform a summary to another locked summary
 +    /// if possible. This is where the concept of a lockfile comes into play.
 +    ///
 +    /// If a summary points at a package id which was previously locked, then we
 +    /// override the summary's id itself, as well as all dependencies, to be
 +    /// rewritten to the locked versions. This will transform the summary's
 +    /// source to a precise source (listed in the locked version) as well as
 +    /// transforming all of the dependencies from range requirements on
 +    /// imprecise sources to exact requirements on precise sources.
 +    ///
 +    /// If a summary does not point at a package id which was previously locked,
 +    /// or if any dependencies were added and don't have a previously listed
 +    /// version, we still want to avoid updating as many dependencies as
 +    /// possible to keep the graph stable. In this case we map all of the
 +    /// summary's dependencies to be rewritten to a locked version wherever
 +    /// possible. If we're unable to map a dependency though, we just pass it on
 +    /// through.
 +    pub fn lock(&self, summary: Summary) -> Summary {
          let pair = self.locked.get(summary.source_id()).and_then(|map| {
              map.get(summary.name())
          }).and_then(|vec| {
Simple merge